perf(reconciler): skip redundant automation-name write when value already matches (P3) by azchohfi · Pull Request #695 · microsoft/microsoft-ui-reactor

azchohfi · 2026-06-26T08:00:10Z

P3 — skip the redundant per-cell automation-name write

Phase-1 finding. Reconciler.UpdateDefaultAutomationName runs on every changed cell that goes through Update. In the steady state the caption-derived default it computes already equals the live UIA Name, so the SetName it issues is a value no-op — pure per-cell overhead.

Change

Extract the decision into a pure, DP-free ResolveDefaultAutomationNameUpdate(current, oldCaption, newCaption) helper (headless-testable in Reactor.Tests).
Add an idempotent-write guard: after computing the trimmed caption-derived default, skip the SetName when the live Name already equals it. GetName still runs for non-whitespace captions — only the redundant same-value write is skipped.
(Behavior-identical micro-skip: a whitespace new caption returns before the GetName read, exactly as main's first line does.)

Correctness (MED-risk area — automation names)

The helper models main's GetName + author-override + SetName logic exactly, plus the idempotent guard:

Author override (live Name ≠ old caption) → left untouched. ✅
Genuine caption change A→B → writes B. ✅
Restore default: a removed .AutomationName() override makes ApplyModifiers clear the Name before this runs; with an unchanged caption the default is still re-applied (empty current ≠ trimmed → write). ✅ (This is the regression a naïve "skip when caption unchanged" cut would have introduced — caught in review, now pinned by a teeth test.)
Steady state (live Name already == default) → skip the redundant write. ✅ (the saving)
>100-char trim preserved. ✅

Tests

ReconcilerAutomationNameTests (12 cases) pin the decision with teeth both ways:

Revert the idempotent guard → the skip test sees a redundant "X" write → fails.
Re-add a blanket unchanged-caption skip → the restore-default test returns null → fails.

Scope / gate

src/Reactor/Core/Reconciler.cs only — file-disjoint from perf(reconciler): skip redundant element-modifier self-merge (P1) #692 (P1) and the rest of the perf fleet.
AOT core-lib build 0W/0E; full Reactor.Tests 9714 passed / 0 failed / 64 skipped.
Honest framing: perf(reconciler): skip redundant element-modifier self-merge (P1) #692 already removed the big alloc chunk. On StocksGrid specifically the guard rarely fires (changed cells have changed captions), so the win is a modest base-cost shave and may read within-noise under /perf — that's fine data (the automation write isn't a StocksGrid bottleneck). The guard still removes provably-redundant writes for any control whose non-caption props change under an unchanged caption.

🔒 DRAFT + merge-gated: does not merge until the improved /perf can resolve its diff and the user gives explicit GO (same rule as #692).

…ed caption (P3) UpdateDefaultAutomationName ran a UIA GetName read + SetName write on every changed cell that goes through Update, even when the caption was unchanged - where the resulting Name write is a value no-op (or hits the author-override guard the GetName already protects). Add a caption-only fast-path that returns before touching the DP when the new caption is empty/whitespace or equals the old caption, and factor the remaining decision into a pure, DP-free ResolveDefaultAutomationNameUpdate helper so the caption/override policy is unit-testable headlessly. On a changed caption the original GetName + author-override + SetName path runs unchanged, so a genuine caption change still flows to UIA and author-set names are never clobbered. Scope is Reconciler.cs only (file-disjoint from the P1 PR #692). New headless tests pin the decision, including a teeth case: reverting the unchanged-caption skip makes the empty-live-Name assertion return the caption instead of null and fail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This PR aims to reduce reconciler update overhead by avoiding unnecessary UIA AutomationProperties.Name read/write round-trips when a cell’s caption hasn’t changed, while keeping the author-override semantics intact. It also factors the decision logic into a pure helper so the policy can be verified with headless unit tests.

Changes:

Added a caption-only fast-path to Reconciler.UpdateDefaultAutomationName to skip UIA DP interactions when newCaption is whitespace or unchanged.
Introduced Reconciler.ResolveDefaultAutomationNameUpdate(current, oldCaption, newCaption) to make the update decision DP-free and unit-testable.
Added ReconcilerAutomationNameTests to pin the intended policy (including “teeth” cases).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
src/Reactor/Core/Reconciler.cs	Adds the unchanged-caption fast-path and a pure helper to decide whether/how to update `AutomationProperties.Name`.
tests/Reactor.Tests/ReconcilerAutomationNameTests.cs	Adds headless tests covering the new helper’s decision policy and invariants around author overrides and trimming.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…guard Addresses Copilot review on #695. The first cut skipped UpdateDefaultAutomationName whenever oldCaption == newCaption. That is unsafe: ApplyModifiers runs BEFORE this (Update.cs:188-200) and can clear AutomationProperties.Name when an explicit .AutomationName() override is removed - even though the caption is unchanged. The blanket skip then left UIA Name empty instead of restoring the caption-derived default (which main does). Replace the unchanged-caption skip with an idempotent-write guard: keep main's GetName + author-override logic verbatim, compute the trimmed target, and skip only the SetName when the live Name already equals it (a value no-op). This still WRITES when the default must be (re)applied - including the cleared-Name-unchanged-caption restore case - so behavior is identical to main minus the redundant same-value write. Tests rewritten: the teeth now pin the idempotent guard (revert it -> the skip test sees a redundant 'X' write and fails) AND the restore-default case (a blanket unchanged-caption skip -> the restore test returns null and fails). Added an author-override-survives-unchanged-caption case. Full Reactor.Tests 9714 passed / 0 failed / 64 skipped; core-lib Release AOT 0W/0E. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

GetName still runs for non-whitespace captions; the P3 saving is the skipped redundant SetName inside the helper. Reword the comment so it no longer implies the GetName read is removed for unchanged captions (Copilot review on #695). Comment-only; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

pr-review (test-coverage L1) flagged that the P3 idempotent-write guard's live UIA seam was only pinned headlessly via the pure helper ResolveDefaultAutomationNameUpdate. The subtle restore-default branch — a removed .AutomationName() override clears the live Name (ApplyModifiers ClearValue) and the guard must re-apply the caption default even though the caption is unchanged — was not proven end-to-end through a real control. Extend the CoreCov_AccessibilityModifiers selftest with a third phase that drops the .AutomationName() override (caption unchanged) and asserts AutomationProperties.GetName restores the caption-derived default, plus assertions that the author override wins at mount and a changed override still flows through. Exercises UpdateDefaultAutomationName via a live WinUI control. Teeth: a blanket unchanged-caption skip (or dropping the live SetName) leaves the Name cleared at phase 2 -> A11y_Name_RestoredToCaptionDefault flips. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Reword the UpdateDefaultAutomationName comment so it attributes the conditional DP access to the SetName *write*, not a vague "touch the DP" — the GetName read always runs for a non-whitespace caption; only the SetName write is skipped when the helper returns null. Comment-only; no behavior change. Addresses the Copilot review thread at Reconciler.cs:2693. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

azchohfi · 2026-06-26T18:55:05Z

/perf

github-actions · 2026-06-26T19:20:28Z

⚡ Reactor perf comparison

Workload: StressPerf.ReactorOptimized StocksGrid · --percent 50 --duration 10 · x64 Release · median of 12 paired runs (2 warmup dropped); Δ is the mean change with a 95% CI · PR head and main built and run interleaved on the same runner.

Regression vs `main` baseline

Metric	`main` (baseline)	This PR	Δ (95% CI)	Status
Renders/sec ↑	2.78	2.78	-0.1% _{95% CI [-5.7, +5.4]}	≈ within noise
Avg Reconcile (ms) ↓	130.5	130.7	-0.9% _{95% CI [-3.3, +1.5]}	≈ within noise
Avg Diff (ms) ↓	118.9	118.2	-1.0% _{95% CI [-3.4, +1.4]}	≈ within noise
Avg Memory (MB) ↓	287.4	287.7	+0.1% _{95% CI [-0.7, +1.0]}	≈ within noise

Low-mutation skip-floor (`--percent 0`)

At --percent 0 the workload mutates few cells per tick (always at least one), so reconcile/diff isolate the O(n) per-tick child skip-walk floor that higher mutation rates dilute — ChildReconciler re-walks every child each tick even when nothing moved. The closer --percent is to 0, the more this floor is the signal, so a structural-skip optimization shows up cleanly where the headline table above buries it. Δ is the mean paired change with a 95% CI.

Metric	`main` (baseline)	This PR	Δ (95% CI)	Status
Renders/sec ↑	16.41	16.84	+3.2% _{95% CI [-3.1, +9.5]}	≈ within noise
Avg Reconcile (ms) ↓	36.4	37.7	+1.6% _{95% CI [-8.4, +11.6]}	≈ within noise
Avg Diff (ms) ↓	34.6	35.8	+1.7% _{95% CI [-9.1, +12.6]}	≈ within noise
Avg Memory (MB) ↓	267.4	267.9	+0.2% _{95% CI [-0.2, +0.5]}	≈ within noise

Allocation (Reactor) — lower is better

Metric	`main` (baseline)	This PR	Δ (95% CI)	Status
Alloc bytes/render ↓	5678978	5717467	+0.2% _{95% CI [-1.2, +1.7]}	≈ within noise
Gen0 GC / 1k renders ↓	200.00	210.59	+6.5% _{95% CI [-7.5, +20.5]}	≈ within noise

Keyed-list workload (`StressPerf.KeyedList`, `--percent 50`)

A separate macro workload: a ~500-row stably keyed list whose rows are reordered / inserted / removed each tick. Because every child carries a key, the child reconciler takes its keyed arm (ReconcileKeyed → ReconcileKeyedMiddle, the LIS-based minimal-move pass) instead of the positional re-walk the StocksGrid tables above measure — so this is the sensitive macro signal for keyed-diff work the positional cells can never reach. Same interleaved paired-Δ 95% CI as the headline table.

Metric	`main` (baseline)	This PR	Δ (95% CI)	Status
Renders/sec ↑	20.52	20.16	-2.1% _{95% CI [-6.2, +1.9]}	≈ within noise
Avg Reconcile (ms) ↓	16.2	16.4	+3.9% _{95% CI [-0.5, +8.4]}	≈ within noise
Avg Diff (ms) ↓	16.0	16.2	+3.9% _{95% CI [-0.6, +8.5]}	≈ within noise
Avg Memory (MB) ↓	168.3	169.0	0.0% _{95% CI [-0.6, +0.6]}	≈ within noise

Allocation (keyed-list) — lower is better

Metric	`main` (baseline)	This PR	Δ (95% CI)	Status
Alloc bytes/render ↓	313792	314083	+0.1% _{95% CI [-0.3, +0.4]}	≈ within noise
Gen0 GC / 1k renders ↓	18.31	16.85	-0.7% _{95% CI [-6.1, +4.7]}	≈ within noise

Reconciler micro-benchmarks (`PerfBench.ControlModel`)

Production --variant Reactor control-model path, ns-resolution and WinUI-undiluted (spec-047 M1–M13) — ↓ lower is better. Status tracks allocated bytes/op, the authoritative signal here; it is deterministic for structurally-fixed benches, while dispatcher / background-thread benches carry a small process-to-process offset, so a bench is flagged only when its 95% CI clears a ±3% minimum-effect band (real structural alloc changes are several percent to many-x). ns/op is shown for context but is not auto-flagged (its paired CI is rep-interleaved but the flag remains dormant pending a real-CI identical-binary band calibration). Δ is the mean paired change with a 95% CI.

Bench	`main` ns/op	Δ ns (95% CI)	`main` B/op	Δ alloc (95% CI)	Status
`M1` Mount_Leaf_NoCallback	148510.0	-0.1% _{95% CI [-1.3, +1.1]}	1140.9	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M2` Mount_Leaf_OneCallback	107607.2	-2.5% _{95% CI [-7.6, +2.5]}	3383.3	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M3` Mount_Leaf_ThreeCallbacks	217610.7	-2.5% _{95% CI [-4.8, -0.3]}	8479.7	-0.4% _{95% CI [-2.7, +1.8]}	≈ within noise
`M4` Dispatch_Switch_Cold	106880.2	-3.9% _{95% CI [-8.1, +0.4]}	1767.8	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M5` Dispatch_Switch_Warm	106501.3	-0.1% _{95% CI [-3.7, +3.4]}	1766.0	-0.8% _{95% CI [-1.8, +0.3]}	≈ within noise
`M6` Dispatch_ExternalType	91672.2	-1.0% _{95% CI [-1.8, -0.1]}	987.6	-1.3% _{95% CI [-3.2, +0.6]}	≈ within noise
`M7` Update_NoChange	55443.4	+0.8% _{95% CI [-0.7, +2.3]}	452.1	+5.5% _{95% CI [-0.8, +11.9]}	≈ within noise
`M8` Update_OneLeafChanged	41849.7	+0.1% _{95% CI [-0.8, +1.0]}	536.0	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M9` Update_AllChanged	2790190.3	+1.5% _{95% CI [+0.7, +2.3]}	184278.1	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M10` EventHandlerState_Alloc	85538.4	+1.5% _{95% CI [-1.4, +4.4]}	3095.2	0.0% _{95% CI [0.0, +0.1]}	≈ within noise
`M11` ModifierEHS_Frequency	45811.2	+0.4% _{95% CI [-1.6, +2.4]}	638.9	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M12` Pool_Rent_HotPath	117703.5	-0.3% _{95% CI [-1.3, +0.7]}	1099.9	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M13` Setters_Suppression_Scope	108.3	+5.6% _{95% CI [-14.5, +25.8]}	26.7	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`M14` Dsl_Rebuild_Cascade	1508448.0	-12.8% _{95% CI [-15.5, -10.2]}	2231828.9	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`C207` ChangeHandler_DpRead_Coalesce	1173.4	+7.5% _{95% CI [-2.5, +17.4]}	0.6	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`OAlloc` Optional_Element_Alloc	214.8	-0.7% _{95% CI [-4.2, +2.8]}	528.0	0.0% _{95% CI [0.0, 0.0]}	≈ within noise
`OUpdate` Optional_Reconciler_Update	12175.1	+1.1% _{95% CI [-1.7, +3.8]}	2772.3	0.0% _{95% CI [0.0, 0.0]}	≈ within noise

Cross-framework reference (same StocksGrid workload)

Metric	vanilla WinUI3¹	Rust `windows-reactor`²	Reactor (this PR)
Renders/sec ↑	3.18	4.80	2.78
Avg Reconcile (ms) ↓	n/a	19.9	130.7
Avg Diff (ms) ↓	n/a	17.9	118.2
Avg Memory (MB) ↓	263.5	196.2	287.7

_{↑ higher is better · ↓ lower is better. Within noise = the 95% confidence interval of the paired Δ includes 0 (no change resolvable at this sample size); ✅ improvement / ⚠️ regression require the CI to exclude 0.}
_{Allocation metrics (alloc bytes/render, Gen0 GC) are the sensitive signal for allocation-reduction work, where the mean-ms / memory figures are largely flat. They read n/a for a harness built from a revision that predates them (rebase the PR onto main to populate them).}
_{Reconciler micro-benchmarks run PerfBench.ControlModel --variant Reactor (M1–M13) as a headless loop bracketed by per-thread alloc + GC counters — ns-resolution and free of WinUI render / working-set dilution, so they resolve Core/Reconciler allocation deltas the macro StocksGrid workload cannot. main and PR each link their own src/Reactor build and are rep-interleaved (a fresh alternated process per rep); Δ is the paired 95% CI over per-rep means. The Status column tracks allocated bytes/op (deterministic for identical code); ns/op is informational — its paired CI is now unbiased but the flag stays dormant pending a real-CI identical-binary band calibration.}
_{¹ vanilla WinUI3 = StressPerf.Direct (imperative; no virtual-DOM, so it has no reconcile/diff phase — those cells read n/a). Measured live on this runner.}
_{² Rust = test_reactor_perf from microsoft/windows-rs — a port of this harness (same StocksGrid, same --percent/--duration CLI). Built from source and measured live on this runner.}
_{Absolute numbers are runner-dependent — trust the Δ vs main, not the absolute values. Memory (working set) is the noisiest metric.}
_{Runner: CPU: AMD EPYC 7763 64-Core Processor · 4 logical cores · 16 GB RAM · runner: GitHub Actions 1043006550.}
_{Generated by .github/workflows/perf-compare.yml · PR 46537b7 vs main e8572a0 · 2026-06-27T07:29:29Z · run log.}

azchohfi · 2026-06-27T05:49:19Z

/perf

azchohfi · 2026-06-27T06:02:55Z

Closing after re-validation on current main (post-#692 + #665). Confirmed an honest no-op: automation-name-churn is below resolution (positional memoized cells rarely re-set an identical allocating automation name); all macro blocks flat, all 17 micros 0.0% deterministic. Reopen if an automation-name-churn micro/workload is added.

azchohfi · 2026-06-27T06:29:39Z

/perf

azchohfi · 2026-06-27T06:50:03Z

/perf

… P4) (#699) * perf(reconciler): structural-skip untouched child ranges (positional, P4) Make `ChildReconciler.ReconcilePositional` O(changed) instead of O(count) when a memoizing producer (`UseMemoCellsByIndex`) reuses untouched cells reference- equal. Targets the positional skip-walk FLOOR — the per-render O(count) cost of visiting every cell to confirm it can be skipped — which dominates low-mutation renders of large keyed grids (e.g. StocksGrid). Mechanism (CWT side-channel hint, mirrors #681's _dirtyAncestorPath bridge): • Producer: the `UseMemoCellsByIndex` reuse branch publishes a `ChildDiffHint` (ChangedIndices + ThemeSensitiveCount) keyed by reference on the fresh-per- render Element[]. No Element-record widening; AOT-safe (ConditionalWeakTable, no reflection). The theme count is carried forward incrementally so steady- state reuse stays O(changed); a one-time O(count) scan runs only on the first reuse after a full rebuild and as a defensive recompute. • Consumer: `ReconcilePositional` engages a fast path that updates ONLY the hinted changed indices and skips the rest, iff ALL hold: 1. old/new element counts match, 2. the live child collection equals that count (no in-flight anim inflated it), 3. no animation ambient, 4. a hint is present for THIS array (a CWT hit also proves Filter returned the same reference — no null/EmptyElement shifted the index space), 5. no cell is theme-sensitive (`!AnyThemeSensitive`), 6. the container is not on #681's dirty-ancestor path. Correctness: • Untouched indices are reference-equal BY CONSTRUCTION (the hook reuses prevChildren[i] for unchanged i and rebuilds only changedIndices). The changed and full-walk paths share a single `UpdateCommonChild` helper, so both honour identical skip / update / type-mismatch semantics. • The theme gate is the load-bearing safety property: the ONLY work the full walk does for an untouched cell that a structural skip would drop is re-resolving `ApplyThemeBindings` / `ApplyResourceOverrides` ThemeRefs against the effective theme (which a parent RequestedTheme toggle can change WITHOUT touching the element tree). Gating on the whole-array `AnyThemeSensitive` flag is provably safe and sidesteps the subtle dirty-path reasoning that bit P2. Tests: • Headless (Reactor.Tests): producer hint correctness incl. incremental theme- count carry + caller-mutation snapshot (UseMemoCellsTests); hint registry + IsThemeSensitive (ChildDiffHintsTests); consumer differential vs full walk incl. the gate teeth `ThemeSensitive_Hint_Forces_Full_Walk` (revert the gate → fails), count-mismatch, defensive OOB, empty-changed (ChildReconcilerStructuralSkipTests). • Live selftests (Reactor.AppTests.Host): LifecycleParity (OnUpdateAction fires for a changed index, never for untouched ref-equal — == full walk); ThemeRangeParity (themed ref-equal range under a RequestedTheme toggle renders + re-themes, no cell dropped). Per the empirical note below, the authoritative gate teeth is the headless visited-index assertion, not a live color delta. Empirical theme note: a LIVE color-delta teeth for the theme gate is impossible — WinUI auto-re-resolves a `{ThemeResource}` Style setter on any effective-theme change even when Reactor structurally skips the cell (verified: gate reverted → cells skipped, ApplyThemeBindings not re-run, yet brushes still went Light→Dark). The one snapshot a skip truly leaves stale (`ApplyResourceOverrides`' concrete ThemeRef.Resolve into fe.Resources) does not reliably re-resolve in the reconcile harness. The headless `ThemeSensitive_Hint_Forces_Full_Walk` is therefore the gate's load-bearing teeth; the live fixture is the end-to-end parity companion. Measurement: the win shows under a low-mutation skip-floor metric (PERFVAL's `--percent 0`); on the default 50%-mutation StocksGrid it is within noise. File-disjoint from the perf fleet (#692/#695 own Reconciler.Update.cs; this touches ChildReconciler.cs / ChildDiffHints.cs / UseMemoCells.cs + a parentControl thread- through in Reconciler.cs / V1HandlerAdapter.cs). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf(reconciler): fold pr-review findings into PR-C structural skip Address the internal pr-review skill + GitHub Copilot findings on the positional structural-skip fast path. No behavior change for the StocksGrid target workload (all new gates pass on it); each fold tightens correctness or documents an invariant. - Hot-reload safety (C1): add `!ForceFullRenderActive` gate so a hot-reload force pass never structurally skips an untouched wrapper cell (the dirty path is empty during a pure force pass, so the dirty-path gate alone did not cover it). Falls back to the full walk, which honours ForceRenderThroughWrapper per cell. - Array-identity guard (S1): the hint now carries a WeakReference to the exact previous-render array its ChangedIndices were diffed against; the fast path engages only when the reconciler's old array IS that array. A cheap, self-documenting sufficient condition for the per-index ref-equality invariant; any defensive copy upstream safely falls back to the full walk. Weak on purpose -- a strong ref would chain every historical array through the reference-keyed CWT and leak. - Duplicate-index hardening (dedupe): snapshot + sort/compact the caller's changedIndices before the theme tally / builder / publish. A duplicated themed->plain index could otherwise under-count the incremental theme-sensitive tally and wrongly publish AnyThemeSensitive=false, and would rebuild + re-update the same cell N times. - Dirty-path gate (T1): documented as conservative defense-in-depth. Proven by experiment that it is behaviorally redundant given the count/CWT/array-id gates (the full walk skips a ref-equal self-triggered cell identically via CanSkipUpdate), retained as cheap insurance; costs nothing on the target workload (cell panel is a descendant, not an ancestor, of the self-triggered grid component). - ResourceOverrides conservatism (C3): documented why the ThemeRef-backed ResourceOverrides arm of IsThemeSensitive is intentionally conservative. Tests: weak-ref round-trip + stale-old-array teeth (gate 8) + chained theme-count carry (T3) + duplicate-index theme-count/build-once (T4). Full Reactor.Tests green (9731); StructuralSkip selftests green; core lib Release AOT-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * perf(reconciler): address Copilot review on PR-C structural skip Fold the three GitHub Copilot review findings on the positional structural-skip fast path. All are hardening; no behavior change on the StocksGrid target. - Null cells (findings 1+2): a cell builder may legitimately return null (ChildReconciler.Filter drops nulls downstream), but PR-C's theme tally now inspects prev/built cells via ChildDiffHints.IsThemeSensitive, which dereferenced element.ThemeBindings and would NRE on a null. Widen the predicate to accept Element? and treat null as non-theme-sensitive (a null has no bindings to re-resolve). Fixes all three call sites in UseMemoCellsByIndex (the O(count) CountThemeSensitive scan + both incremental tally reads) at the single chokepoint. - DebugElementsSkipped diagnostic (finding 3): the fast path adjusted the skipped-element counter by `common - changed.Length`, but the loop defensively ignores out-of-range hint indices, so the raw hint length over-counts visited work and the diagnostic could skew (or, with enough out-of-range indices, go negative). Track indices ACTUALLY visited and base the adjustment on that, making the counter match the full walk exactly. Tests: null-cell predicate guard + producer null-cell theme-scan teeth; the out-of-range consumer test now asserts the skipped-element count equals the full-walk total (4), which the old `common - changed.Length` undercounted. Full Reactor.Tests green (9733); core lib Release AOT-clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fold pr-review skill findings into PR-C structural skip Ran the internal pr-review skill on the PR-C HEAD (7 dimensions + a gpt-5.4 multi-model cross-check, a different model family). Fold the actionable findings: - H1 (test-coverage; multi-model CONFIRMED load-bearing): add a hot-reload gate teeth selftest. StructuralSkip_HotReloadWrapperReRender puts a WRAPPER cell (Component) inside a UseMemoCellsByIndex range whose body a simulated hot-reload edit changes, then drives a real force pass. The fast path's !ForceFullRenderActive gate must defer to the full walk (which honours ForceRenderThroughWrapper per cell) so the wrapper re-renders its edited body. Teeth verified: reverting the gate fails WrapperReRenders + OldBodyGone (the structural skip swallows the edit). - H2 (test-coverage; partially-confirmed): add a headless differential test that mirrors the real producer's reference-equal reuse at untouched indices (not fresh copies) and asserts the fast-path output == full-walk output (identical skip accounting, no structural mutation, visited set a subset). - M1 (security + correctness; multi-model: real but not a ship-blocker, no cheap complete defense) + M3 (docs + api): document the returned array's immutability / no-mutation contract, the changedIndices dedupe contract, and the theme-sensitive fallback in the UseMemoCellsByIndex XML doc; hand-sync the generated reference MD. Dispositions recorded (no code change): - H3 / gate 6 (!IsOnDirtyAncestorPath): multi-model DISPUTED the test-coverage finding and independently confirmed the gate is behaviorally redundant given the count/CWT/array-id gates (a ref-equal untouched cell is skipped identically by the full walk via Element.CanSkipUpdate before dirty-path logic is consulted). No behavioral teeth is constructible; kept as documented cheap defense-in-depth. - M2: ThemeRangeParity already documents itself in-code as a smoke/parity check, not the gate teeth; the authoritative !AnyThemeSensitive teeth is the headless ChildReconcilerStructuralSkipTests.ThemeSensitive_Hint_Forces_Full_Walk. - L1: the ResourceOverrides arm of IsThemeSensitive is intentionally conservative (already documented) per the verified theme crux. Gates: core lib Release AOT 0W/0E; Reactor.Tests 9734 pass / 0 fail; StructuralSkip selftests 3 fixtures / 14 checks green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: pin structural-skip per-cell read elision as an allocation budget The existing ChildReconcilerStructuralSkipTests assert the fast path's VISIT COUNT (which child indices are read) but nothing pins the resulting allocation cut, so the measured StocksGrid allocation win (#699) could be silently reverted with every behavioural test still green. Add Structural_Skip_Pins_PerCell_Read_Elision_As_Allocation_Budget: a MeasuringChildCollection charges a fixed managed allocation per Get(i), modeling the per-cell COM read / marshaling the skip elides for untouched reference-equal cells (the real cost is native and unmeasurable headless). Fast path (hint published) allocates O(changed); full walk (no hint) allocates O(count). Asserts the mechanism (5 vs 500 reads/iter) and an 8x GC-bytes budget. Has teeth: disabling the fast-path gate makes the hinted path walk every cell, collapsing fast onto full and failing the test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * harden ChildDiffHint.AnyThemeSensitive to fail-safe (!= 0 not > 0) Copilot review on #699 flagged that AnyThemeSensitive derives from ThemeSensitiveCount > 0, so a hypothetical negative count would read as NOT theme-sensitive and could allow the structural-skip fast path to skip a theme-sensitive subtree (a missed-update risk). The only producer (UseMemoCellsByIndex) already clamps its incremental tally to a >= 0 floor before publishing (UseMemoCells.cs:299-300) and CountThemeSensitive only counts upward, so a negative is unreachable and > 0 is correct today. But this is the SAFETY gate for a correctness- sensitive skip, so harden the type to be fail-safe regardless: test != 0 rather than > 0. Behavior is byte-identical for every value the producer can emit (all >= 0); the only difference is that an anomalous negative now BLOCKS the skip (forces the always-correct full walk) instead of silently allowing it — the correct fail direction for a correctness gate. Provably perf-neutral: the StocksGrid workload publishes count == 0 every render, where both > 0 and != 0 yield false identically, so the fast path engages unchanged. Adds a fail-safe teeth test that goes red if the guard is reverted to > 0. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: azchohfi <azchohfi@users.noreply.github.com>

azchohfi requested a review from Copilot June 26, 2026 08:00

Copilot started reviewing on behalf of azchohfi June 26, 2026 08:00 View session